feat(client-core): Introduce formating API#10653
Conversation
…cale option Replace hardcoded en-US locale with Intl.NumberFormat-derived locale that resolves decimal/thousands separators and currency symbol position from the runtime. Add locale option to FormatValueOptions, cached per locale:currency pair. Extract FormatValueMember type.
github-actions
bot
added
client:core
|
Claude finished @ovr's task in 2m 59s —— View job PR Review:
|
| Priority | Issue | Location |
|---|---|---|
| 🔴 High | Export is commented out — consumers can't use formatValue. Needs clarity on the access strategy. |
index.ts:913-914 |
| 🟡 Medium | parseNumber returns NaN for non-numeric strings — d3 will render literal "NaN" in the UI. Should handle gracefully. |
format.ts:70-76 |
| 🟡 Medium | Hardcoded grouping: [3] ignores locales with non-uniform grouping (e.g., en-IN uses [3, 2]). |
format.ts:24-26 |
| 🟢 Low | getCurrentLocale() allocates a new Intl.NumberFormat on every call — can be cached. |
format.ts:41-43 |
| 🟢 Low | Currency code leaks into cache key for non-currency formats, causing duplicate cache entries. | format.ts:119-122 |
| 🟢 Low | Missing test edge cases: NaN input, invalid dates, custom emptyPlaceholder, very large numbers. |
format.test.ts:84 |
Security
No concerns — this is a pure formatting utility with no network calls, DOM manipulation, or dynamic code execution.
Verdict
The core logic is sound and well-tested for happy paths. The commented-out export is the most important question — it determines whether this feature is actually shippable in this PR or a foundation for a follow-up. The NaN handling should also be addressed before merge to avoid rendering "NaN" in production UIs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // We don't export it for now, because size of builds for cjs/umd users will be affected | ||
| // export * from './format'; |
There was a problem hiding this comment.
The export is commented out — meaning consumers can't actually use formatValue via the standard @cubejs-client/core import.
Is this intentional for this PR? If so, how are consumers expected to access this function? If this is meant to be usable in a follow-up (e.g., only from ESM builds), it might be worth adding a note in the PR description about the plan.
If tree-shaking is the concern, modern bundlers (Rollup, webpack, esbuild) will eliminate unused exports from ESM builds. The CJS/UMD concern is valid, but you could consider a separate entry point (e.g., @cubejs-client/core/format) instead of blocking the export entirely.
There was a problem hiding this comment.
Intended for now, I will prepare separate entrypoints in different PR, It require testing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10653 +/- ##
=======================================
Coverage 57.84% 57.84%
=======================================
Files 215 215
Lines 16609 16609
Branches 3336 3336
=======================================
Hits 9607 9607
Misses 6514 6514
Partials 488 488
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @ovr's task in 2m 47s —— View job PR Review:
|
| Priority | Issue | Location |
|---|---|---|
| 🔴 High | Export is commented out — consumers can't use formatValue. Needs clarity on the access strategy or a note in the PR description. |
index.ts:913-914 |
| 🟡 Medium | parseNumber returns NaN for non-numeric strings — d3 will render literal "NaN" in the UI. Should return null and fall back to emptyPlaceholder. |
format.ts:78-84 |
| 🟡 Medium | new Date() on invalid input produces Invalid Date — timeFormat silently outputs "NaN" placeholders for each directive. Should validate and fall back. |
format.ts:142 |
| 🟢 Low | Currency code leaks into cache key for non-currency formats, causing duplicate cache entries for percent and number formats. |
format.ts:128-130 |
| 🟢 Low | Missing test edge cases: NaN input, invalid dates, custom emptyPlaceholder, empty string with numeric format. |
format.test.ts:84 |
Security
No concerns — this is a pure formatting utility with no network calls, DOM manipulation, or dynamic code execution.
Verdict
The core logic is sound and well-tested for happy paths. The commented-out export is the most important question — it determines whether this feature is actually shippable or a foundation for a follow-up. The NaN/invalid date handling should be addressed before merge to avoid rendering "NaN" in production UIs — these are the most likely edge cases users will hit with real-world data.
| // We don't export it for now, because size of builds for cjs/umd users will be affected | ||
| // export * from './format'; |
There was a problem hiding this comment.
The export is still commented out — consumers can't access formatValue via the standard @cubejs-client/core import.
Is this intentional for this PR? If the concern is bundle size for CJS/UMD users, a separate entry point (e.g., @cubejs-client/core/format) could expose it without affecting existing builds. Alternatively, modern bundlers tree-shake unused ESM exports effectively.
If this is meant to land without the export, it might be worth noting in the PR description that this is a foundational PR with a follow-up planned to wire the export.
There was a problem hiding this comment.
Intended for now, I will prepare separate entrypoints in different PR, It require testing
ovr
force-pushed
the
feat/client-core-formatting
branch
from
79011b2 to
ec061d9
Compare
ovr
changed the title
4 participants
No description provided.